Clean up pagination handling in Unified Data Table #4
Conversation
| setCurrentPageIndex(pageIndex); | ||
| onUpdatePageIndex?.(pageIndex); |
There was a problem hiding this comment.
| setCurrentPageIndex(pageIndex); | |
| onUpdatePageIndex?.(pageIndex); | |
| const calculatedPageIndex = pageIndex > pageCount - 1 ? 0 : pageIndex | |
| setCurrentPageIndex(calculatedPageIndex); | |
| onUpdatePageIndex?.(calculatedPageIndex); |
I think this implementation could be updated to fix the bug you found by calculating the page index within this callback...
There was a problem hiding this comment.
@davismcphee , i do not think calculating there is needed, because onChangePage is not called when we need new pageIndex to be calculated. The other useEffect should be enough and i have included that.
There was a problem hiding this comment.
Also, I think onUpdatePageIndex should now be solely based on currentPageIndex. Any changes to that should be automatically trigger onUpdatePageIndex instead of calling onUpdatePageIndex at multiple places.
| pageIndex: pagination.pageIndex > pageCount - 1 ? 0 : pagination.pageIndex, | ||
| pageSize: pagination.pageSize, | ||
| pageSizeOptions: rowsPerPageOptions ?? getRowsPerPageOptions(pagination.pageSize), | ||
| pageIndex: currentPageIndex > pageCount - 1 ? 0 : currentPageIndex, |
There was a problem hiding this comment.
| pageIndex: currentPageIndex > pageCount - 1 ? 0 : currentPageIndex, | |
| pageIndex: currentPageIndex, |
...Then passing currentPageIndex directly here.
And we'd only need to manually sync when pageCount changes in a separate effect:
useEffect(() => {
setCurrentPageIndex((previousPageIndex) => {
const calculatedPageIndex = previousPageIndex > pageCount - 1 ? 0 : previousPageIndex;
if (calculatedPageIndex !== previousPageIndex) {
onUpdatePageIndex?.(calculatedPageIndex);
}
return calculatedPageIndex;
});
}, [onUpdatePageIndex, pageCount]);
Summary
This PR cleans up the pagination handling in Unified Data Table to make the logic easier to follow and less dependent on side effects.